-
Notifications
You must be signed in to change notification settings - Fork 32
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Handle invalid api key response #56
base: main
Are you sure you want to change the base?
Conversation
if let Some(ref result) = result { | ||
if result.starts_with("Max rate limit reached") { | ||
return Err(EtherscanError::RateLimitExceeded); | ||
} else if result.to_lowercase() == "invalid api key" { | ||
} | ||
if result.to_lowercase().contains("invalid api key") { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
could you please submit this change separately?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hey @mattsse, I'd be happy to but the reality is that it is a dead case without the change to the deserializer. The test case that I added and the original case where result == "invalid api key"
can never be hit since it would deserialize as a ResponseData::Success
case rathere than ResponseData::Error
Let me know if you would still like it as a separate change?
crates/block-explorers/src/lib.rs
Outdated
#[test] | ||
fn can_parse_etherscan_mainnet_invalid_api_key() { | ||
let err = json!({ | ||
"status":"0", | ||
"message":"NOTOK", | ||
"result":"Missing/Invalid API Key" | ||
}); | ||
let resp: ResponseData<Address> = serde_json::from_value(err).unwrap(); | ||
assert!(matches!(resp, ResponseData::Error { .. })); | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this test succeeds on main, so still unclear what's the motivation for this
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
My mistake with writing the test.
It should have been
let resp: ResponseData<String> = serde_json::from_value(err).unwrap();
or ResponseData<Option<String>>
like in the contract_abi case I tested on.
I've updated the test but you will see it should fail on main without the updated serializer.
The problem is that the deserializer can match ResponseData of string/option string as a success or an error case. Since success takes precedence it deserializes as a success.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hey @mattsse, just flagging my above comment this is still an issue and the broken case isn't solved.
I see you opted to make your own PR so I'll leave this one unless you would like the change I made in which case I can update it for you or make any updates you might want.
3208751
to
62545d9
Compare
Hi there 👋🏼
I found an issue when calling contract_abi, where the error returned was a serde error when it should have been an InvalidApiKey error.
This PR